Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: paymaster #907

Merged
merged 9 commits into from
Nov 12, 2024
Merged

fix: paymaster #907

merged 9 commits into from
Nov 12, 2024

Conversation

Marchand-Nicolas
Copy link
Collaborator

@Marchand-Nicolas Marchand-Nicolas commented Nov 5, 2024

close: #905
close: #906

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Integrated typed data signing for a more streamlined signature process.
    • Added a new hook to manage network validation, enhancing user feedback for network status.
    • Improved loading state management for coupon processing based on network conditions.
  • Bug Fixes

    • Improved error handling and logging for signing and transaction execution.
    • Adjusted TypeScript error handling in multiple components for better stability.
    • Enhanced connection management logic in the navbar for improved reliability.
  • Chores

    • Updated dependencies to the latest compatible versions for improved performance and security.

Copy link

vercel bot commented Nov 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 11, 2024 0:00am
sepolia-app-starknet-id ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 11, 2024 0:00am

@Marchand-Nicolas Marchand-Nicolas added the 🚧 In progress do not merge Pull Request in progress, please do not merge label Nov 5, 2024
Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Walkthrough

The changes in this pull request focus on updating the usePaymaster hook within hooks/paymaster.tsx, integrating the useSignTypedData hook for improved transaction signing. Key modifications include replacing the deploymentTypedData state variable with typedData, enhancing error handling, and refining transaction execution logic. The connectWallet function across various components has been updated to suppress TypeScript errors. Additionally, several dependencies in package.json have been updated to their latest versions, ensuring compatibility with the latest libraries.

Changes

File Change Summary
hooks/paymaster.tsx - Integrated useSignTypedData hook.
- Replaced deploymentTypedData with typedData state variable.
- Updated handleRegister to use signTypedDataAsync for signing.
- Adjusted registration logic for clarity.
- Improved error handling with consistent console logging.
- Updated method signature for usePaymaster without parameter changes.
components/UI/connectButton.tsx - Modified connectWallet function to include @ts-ignore for TypeScript error suppression.
components/UI/navbar.tsx - Enhanced connectWallet function with error handling to retry connection on errors (excluding user rejections).
- Added TypeScript ignore comments.
pages/identities.tsx - Updated connectWallet function to include @ts-ignore for TypeScript error suppression.
pages/solana.tsx - Modified connectWallet function to include @ts-ignore for TypeScript error suppression.
components/discount/freeRegisterCheckout.tsx - Introduced useIsWrongNetwork hook to manage network validation state.
- Updated coupon loading logic based on network state.
hooks/isWrongNetwork.tsx - Added new hook useIsWrongNetwork to determine incorrect network status based on chain ID.
pages/_app.tsx - Simplified chains array initialization to include both mainnet and sepolia without conditional logic.
package.json - Updated versions of several dependencies: @starknet-react/chains, @starknet-react/core, big.js, and starknetkit.

Assessment against linked issues

Objective Addressed Explanation
Don't use the free coupon if the network is not correct (#905)
Paymaster impossible to use in production (#906)

Possibly related PRs

  • fix: failing to fetch deployment data #895: The changes in hooks/isDeployed.tsx may relate to the overall transaction execution flow and error handling improvements in the main PR, as both involve handling deployment data and enhancing error management in wallet interactions.

Suggested reviewers

  • fricoben

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between eff3cf3 and 326e1df.

📒 Files selected for processing (1)
  • hooks/paymaster.tsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • hooks/paymaster.tsx

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between c15caf3 and cf54199.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (1)
  • hooks/paymaster.tsx (4 hunks)

hooks/paymaster.tsx Show resolved Hide resolved
hooks/paymaster.tsx Outdated Show resolved Hide resolved
hooks/paymaster.tsx Outdated Show resolved Hide resolved
hooks/paymaster.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
components/UI/navbar.tsx (2)

75-76: Consider investigating proper TypeScript types instead of using ignore comments.

While @ts-ignore comments work as a temporary solution, they bypass TypeScript's type safety. Consider:

  1. Checking if there's a newer version of @starknet-react/core with updated types
  2. Creating proper type definitions for the connectAsync method

Line range hint 75-85: Well-implemented error handling with room for improvement.

The error handling logic effectively distinguishes between user rejections and other errors, with appropriate retry behavior. Consider enhancing the error logging to provide more context about the specific failure:

     } catch (e) {
-      console.error(e);
+      console.error('Wallet connection failed:', {
+        error: e,
+        connector: connector.id,
+        isUserRejection: (e as Error).name === "UserRejectedRequestError"
+      });
       const error = e as Error;
       if (error.name !== "UserRejectedRequestError") connectWallet(connector);
     }
pages/solana.tsx (1)

306-307: Enhance error handling around wallet connection.

The wallet connection logic could benefit from more robust error handling and user feedback.

Consider these improvements:

  1. Replace console.log with proper error tracking
  2. Add user-friendly error messages
  3. Ensure error states are properly reset
 const connectWallet = async (connector: Connector) => {
+  try {
     // eslint-disable-next-line @typescript-eslint/ban-ts-comment
     // @ts-ignore
     await connectAsync({ connector });
     localStorage.setItem("SID-connectedWallet", connector.id);
     localStorage.setItem("SID-lastUsedConnector", connector.id);
+  } catch (error) {
+    // Add proper error tracking
+    console.error("Failed to connect wallet:", error);
+    // Add user-friendly error notification
+    // Reset any relevant error states
+  }
 };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cf54199 and e2e98c5.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (6)
  • components/UI/connectButton.tsx (1 hunks)
  • components/UI/navbar.tsx (1 hunks)
  • hooks/paymaster.tsx (4 hunks)
  • package.json (2 hunks)
  • pages/identities.tsx (1 hunks)
  • pages/solana.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • hooks/paymaster.tsx
🔇 Additional comments (4)
components/UI/connectButton.tsx (1)

23-25: 🛠️ Refactor suggestion

Investigate alternatives to @ts-ignore.

While the type suppression allows the code to compile, it bypasses TypeScript's type safety mechanisms. This could hide potential runtime issues.

Let's verify the type definitions and usage:

Consider these alternatives:

  1. Update type definitions if they're outdated
  2. Use proper type assertions if the types are correct but TypeScript inference is failing
  3. If this is a known issue with the library, add a comment linking to the relevant issue/PR

Example of a type-safe approach if the connector type is known:

await connectAsync({ connector: connector as StarknetConnector });
✅ Verification successful

Let me gather more information about the type definitions and imports to better understand the context.


Based on the gathered information, I can now provide the final response:

The @ts-ignore directive is actually warranted in this case.

The type mismatch occurs due to version compatibility between @starknet-react/core v3.5.0 and starknetkit v2.3.3. The Connector type from starknetkit and the connector type expected by connectAsync from @starknet-react/core have slight differences, but they are functionally compatible at runtime.

This is a known integration pattern when using these specific versions together, and the type suppression is acceptable here since:

  1. The code follows the same pattern across multiple files (solana.tsx, identities.tsx, navbar.tsx)
  2. The functionality works correctly at runtime
  3. Both libraries are actively maintained but have this type impedance mismatch
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check how connectAsync is used elsewhere in the codebase
rg -A 3 "connectAsync.*connector" 

# Look for type definitions or interfaces related to connectAsync
ast-grep --pattern 'interface $_ {
  connectAsync$_: $_
  $$$
}'

Length of output: 2324


Script:

#!/bin/bash
# Check imports and type usage in the connect button component
cat components/UI/connectButton.tsx

# Look for type definitions in the project
rg -g "*.d.ts" "connectAsync|Connector"

# Check package.json for starknet dependencies
cat package.json | grep -A 5 -B 5 "starknet"

Length of output: 3565

package.json (2)

30-30: LGTM! Patch version updates.

The patch version updates for big.js (6.2.1 -> 6.2.2) and starknetkit (2.3.0 -> 2.3.3) are safe improvements that should not introduce breaking changes.

Also applies to: 50-50


25-26: Verify breaking changes in @starknet-react packages.

The significant version jump in @starknet-react/core (3.0.3 -> 3.5.0) warrants verification of breaking changes, especially since it's used for the paymaster functionality.

✅ Verification successful

Let me gather more information about the paymaster hook implementation to verify the usage of the updated dependency.


Let me check the usage of the paymaster hook in the component to complete the verification.


The package updates are safe to proceed with.

The code changes show that the update to @starknet-react/core was specifically needed to fix the paymaster functionality by:

  1. Adding the useSignTypedData hook import
  2. Properly configuring the hook with skipDeploy parameter
  3. Simplifying the signature process by using the new hook instead of direct account.signMessage

The changes are intentional and align with the PR's objective to fix the paymaster functionality. No breaking changes are evident in the implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for breaking changes in the updated packages

# Test: Search for any migration guides or breaking changes in the repository
rg -i "breaking|migration|upgrade" $(fd -t f "CHANGELOG.md|README.md")

# Test: Check if the updated useSignTypedData hook is used correctly
ast-grep --pattern 'useSignTypedData'

Length of output: 256


Script:

#!/bin/bash
# Check the implementation of the paymaster hook
cat hooks/paymaster.tsx

# Search for any other files using the paymaster hook
rg -l "usePaymaster"

# Look for any test files related to paymaster
fd -t f "paymaster.*test|test.*paymaster"

Length of output: 8376


Script:

#!/bin/bash
# Check how the paymaster hook is used in the component
cat components/discount/freeRegisterCheckout.tsx

# Check if there are any other components using useSignTypedData directly
rg -l "useSignTypedData"

# Look for any recent changes in these files
git log -p hooks/paymaster.tsx components/discount/freeRegisterCheckout.tsx

Length of output: 31198

pages/solana.tsx (1)

306-307: 🛠️ Refactor suggestion

Consider addressing the TypeScript type mismatch instead of using @ts-ignore.

The added TypeScript ignore comments suggest there's a type incompatibility between the Connector type and what connectAsync expects. While this works as a temporary solution, it bypasses type checking which could mask potential issues.

Let's verify the type incompatibility:

Consider these alternatives:

  1. Update the Connector type to match what connectAsync expects
  2. Create a type assertion function to safely convert between types
  3. If this is a known issue with @starknet-react/core, consider opening an issue or checking if there's an existing fix

Example of a type assertion approach:

const assertConnector = (connector: Connector): ConnectorType => {
  // Add runtime checks here to ensure type safety
  return connector as ConnectorType;
};

// Then use it in connectWallet
await connectAsync({ connector: assertConnector(connector) });

pages/identities.tsx Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (6)
hooks/isWrongNetwork.tsx (2)

1-5: Consider reordering imports following common conventions.

Consider reorganizing imports to follow the common convention:

  1. React core imports
  2. Third-party library imports
  3. Local utility imports
-import { mainnet } from "@starknet-react/chains";
-import { bigintToStringHex } from "@/utils/stringService";
-import { sepolia } from "@starknet-react/chains";
-import { useNetwork } from "@starknet-react/core";
-import { useEffect, useState } from "react";
+import { useEffect, useState } from "react";
+import { useNetwork } from "@starknet-react/core";
+import { mainnet, sepolia } from "@starknet-react/chains";
+import { bigintToStringHex } from "@/utils/stringService";

1-25: Consider architectural improvements for better maintainability and performance.

  1. Consider creating a centralized network configuration file to manage network-specific constants and environment variables.
  2. Implement memoization using useMemo for the return value to prevent unnecessary re-renders in consuming components.
  3. Consider integrating with error boundaries for graceful error handling of network-related issues.

Example network configuration:

// config/networks.ts
export const NETWORKS = {
  MAINNET: {
    id: mainnet.id,
    name: 'Mainnet',
  },
  TESTNET: {
    id: sepolia.id,
    name: 'Sepolia',
  },
} as const;

export const getExpectedNetwork = () => 
  process.env.NEXT_PUBLIC_IS_TESTNET === "true" 
    ? NETWORKS.TESTNET 
    : NETWORKS.MAINNET;
pages/_app.tsx (1)

Line range hint 59-63: Consider chain-specific RPC configurations.

The current implementation uses a single RPC URL for both chains. Consider using chain-specific RPC URLs for better reliability and network separation.

 const providers = jsonRpcProvider({
   rpc: (_chain: Chain) => ({
-    nodeUrl: process.env.NEXT_PUBLIC_RPC_URL as string,
+    nodeUrl: _chain.id === mainnet.id
+      ? (process.env.NEXT_PUBLIC_MAINNET_RPC_URL as string)
+      : (process.env.NEXT_PUBLIC_TESTNET_RPC_URL as string),
   }),
 });
components/discount/freeRegisterCheckout.tsx (3)

Line range hint 155-169: Consider adding network-specific error message

While the early return for wrong network is good, users might benefit from knowing why their coupon validation stopped.

Consider updating the error handling:

-    if (isWrongNetwork) return setLoadingCoupon(false);
+    if (isWrongNetwork) {
+      setCouponError("Please switch to the correct network before validating coupon");
+      return setLoadingCoupon(false);
+    }

Line range hint 211-224: Consider refactoring disabled state conditions

The disabled state logic is correct but could be more maintainable.

Consider extracting the conditions into a separate function:

const isButtonDisabled = () => {
  const conditions = {
    wrongNetwork: isWrongNetwork,
    domainMinting: domainsMinting.get(encodedDomain),
    noAccount: !account,
    noCoupon: !coupon,
    noDuration: !durationInDays,
    noTarget: !targetAddress,
    termsNotAccepted: !termsBox,
    hasCouponError: Boolean(couponError),
    isLoading: loadingCoupon || loadingGas || loadingDeploymentData || loadingTypedData
  };
  
  return Object.values(conditions).some(Boolean);
};

Then use it in the button:

-  disabled={
-    isWrongNetwork ||
-    (domainsMinting.get(encodedDomain) as boolean) ||
-    !account ||
-    !coupon ||
-    !durationInDays ||
-    !targetAddress ||
-    !termsBox ||
-    Boolean(couponError) ||
-    loadingCoupon ||
-    loadingGas ||
-    loadingDeploymentData ||
-    loadingTypedData
-  }
+  disabled={isButtonDisabled()}

Line range hint 225-244: Consider refactoring button text logic

The nested ternary operators make the button text logic hard to read and maintain.

Consider using a function to determine button text:

const getButtonText = () => {
  if (isWrongNetwork) return "Wrong Network";
  if (!termsBox) return "Please accept terms & policies";
  if (couponError || !coupon) return "Enter a valid Coupon";
  if (loadingCallData) return "Loading call data";
  if (loadingGas) return invalidTx ? txError?.short : "Loading gas";
  if (loadingTypedData) return "Building typed data";
  if (loadingDeploymentData) {
    return paymasterRewards.length > 0
      ? "Loading deployment data"
      : "No Paymaster reward available";
  }
  return "Register my domain";
};

Then use it in the button:

-  {isWrongNetwork
-    ? "Wrong Network"
-    : !termsBox
-    ? "Please accept terms & policies"
-    : couponError || !coupon
-    ? "Enter a valid Coupon"
-    : loadingCallData
-    ? "Loading call data"
-    : loadingGas
-    ? invalidTx
-      ? txError?.short
-      : "Loading gas"
-    : loadingTypedData
-    ? "Building typed data"
-    : loadingDeploymentData
-    ? paymasterRewards.length > 0
-      ? "Loading deployment data"
-      : "No Paymaster reward available"
-    : "Register my domain"}
+  {getButtonText()}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 66f6032 and eff3cf3.

📒 Files selected for processing (5)
  • components/UI/navbar.tsx (3 hunks)
  • components/discount/freeRegisterCheckout.tsx (6 hunks)
  • hooks/isWrongNetwork.tsx (1 hunks)
  • hooks/paymaster.tsx (5 hunks)
  • pages/_app.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • components/UI/navbar.tsx
  • hooks/paymaster.tsx
🔇 Additional comments (3)
hooks/isWrongNetwork.tsx (1)

25-25: LGTM!

The export statement follows React conventions for custom hooks.

pages/_app.tsx (1)

58-58: LGTM! The chains array modification improves network validation.

Including both chains enables proper network detection and validation, which is a good practice for preventing users from connecting to the wrong network.

Let's verify the RPC configuration:

components/discount/freeRegisterCheckout.tsx (1)

24-24: LGTM: Clean hook integration

The integration of useIsWrongNetwork hook follows React best practices with clear naming and proper import path.

Also applies to: 64-64

hooks/isWrongNetwork.tsx Show resolved Hide resolved
hooks/isWrongNetwork.tsx Show resolved Hide resolved
hooks/isWrongNetwork.tsx Show resolved Hide resolved
@Marchand-Nicolas Marchand-Nicolas added 🔥 Ready for review This pull request needs a review and removed 🚧 In progress do not merge Pull Request in progress, please do not merge labels Nov 6, 2024
Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested with braavos works well but with argent I have this on mainnet
Capture d’écran 2024-11-10 à 17 57 17

  • some questions

components/UI/connectButton.tsx Show resolved Hide resolved
@@ -72,6 +71,8 @@ const Navbar: FunctionComponent = () => {

const connectWallet = async (connector: Connector) => {
try {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you add that everywher e?

@fricoben fricoben added ❌ Change request Change requested from reviewer and removed 🔥 Ready for review This pull request needs a review labels Nov 10, 2024
@Marchand-Nicolas
Copy link
Collaborator Author

tested with braavos works well but with argent I have this on mainnet Capture d’écran 2024-11-10 à 17 57 17

Yes there is an issue with Argent currently. The AVNU team is in company retreat in Thailande, so I'm waiting for them to fix the error encountered. For now, I replaced paymaster by a traditional tx for argent, isn't it working ?

@Marchand-Nicolas Marchand-Nicolas added 🔥 Ready for review This pull request needs a review and removed ❌ Change request Change requested from reviewer labels Nov 10, 2024
Copy link
Contributor

@fricoben fricoben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@fricoben fricoben merged commit 29f2323 into testnet Nov 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 Ready for review This pull request needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paymaster impossible to use in prod Don't use the free coupon is network is not the good one
2 participants